1. Isn't this a privileged thing? Can this be used in a bad way?
2. Can this conflict with firejail?
3. We have a checklist in CONTRIBUTING.md for adding new commands, please follow it.

1. Isn't this a privileged thing? Can this be used in a bad way?

I doubt it, since it only uses an unprivileged aa_stack_onexec thing (which is more secure than aa_change_onexec since it doesn't allow transitions to less-restricted domains) and it only allows transitioning to a profile that is already loaded to the kernel -- it doesn't allow loading new profiles.

2. Can this conflict with firejail?

AFAIK, no -- the original setting (as well as firejail-default) profile remains unchanged, except for the fact that it now uses profile stacking instead of original transition.

3. We have a checklist in CONTRIBUTING.md for adding new commands, please follow it.

OK, sorry!

On the checklist:

Update manpages: firejail(1) and firejail-profile(5)

Done

Update shell completions
Update vim syntax files

I doubt it's needed -- the apparmor command already exists, I just added an optional argument to it.

Update --help

Already done.

I just added an optional argument to it.

At least for the zsh completion this makes a differences. For bash and vim you need to check. You can look at other commands with optional arguments like --private.

I just added an optional argument to it.

At least for the zsh completion this makes a differences. For bash and vim you need to check. You can look at other commands with optional arguments like --private.

For Zsh -- done.

kmk3 requested changes 5 hours ago

I wonder, since apparmor_profile is a profile name and not a path, are any special checks needed? If a profile is non-existent, the domain transition will fail and Firejail will show an error (the same behavior as mainline Firejail if firejail-default isn't loaded).

@ChrysoliteAzalea commented on Jul 26:

kmk3 requested changes 5 hours ago

I wonder, since apparmor_profile is a profile name and not a path, are
any special checks needed? If a profile is non-existent, the domain
transition will fail and Firejail will show an error (the same behavior as
mainline Firejail if firejail-default isn't loaded).

Does AppArmor always take the profile name literally or does it do any form of
special processing?

Even if taken literally, maybe it could be an issue if AppArmor does something
like this:

snprintf(buf, sizeof(buf), "/proc/apparmor/foo/%s", profile);

And the profile string contains something like /../../../tmp/evil.

I would hope that AppArmor does some basic validation, but if something is to
be checked, maybe forbiding "/" in the string + calling
reject_metachars(profile, 0) wouldn't hurt...

Though I see that some profiles have multiple levels of nesting in
/etc/apparmor.d. For example, given the following profile:

• /etc/apparmor.d/abstractions/ubuntu-browsers.d/chromium-browser

How would it be referenced in the profile string? Simply "chromium-browser" or
"abstractions/ubuntu-browsers.d/chromium-browser"?

If the latter is possible, then maybe forbid just "../" and "/.." + call
reject_metachars(profile, 0).

How would it be referenced in the profile string? Simply "chromium-browser" or "abstractions/ubuntu-browsers.d/chromium-browser"?

If the latter is possible, then maybe forbid just "../" and "/.." + call reject_metachars(profile, 0).

Thanks, I'll try that. The problem is, when the profile name is auto-generated by AppArmor, it's in the form of the path to file (for example, the profile for mpv will be named "/usr/bin/mpv" and stored in "/etc/apparmor.d/usr.bin.mpv"). The profile name supplied by user is passed to the aa_stack_onexec function. I'll do some testing to see if using this function can lead so some unwanted behavior.

@ChrysoliteAzalea commented on Jul 26:

kmk3 requested changes 5 hours ago

I wonder, since apparmor_profile is a profile name and not a path, are
any special checks needed? If a profile is non-existent, the domain
transition will fail and Firejail will show an error (the same behavior as
mainline Firejail if firejail-default isn't loaded).

Does AppArmor always take the profile name literally or does it do any form of special processing?

Even if taken literally, maybe it could be an issue if AppArmor does something like this:

snprintf(buf, sizeof(buf), "/proc/apparmor/foo/%s", profile);

And the profile string contains something like /../../../tmp/evil.

I would hope that AppArmor does some basic validation, but if something is to be checked, maybe forbiding "/" in the string + calling reject_metachars(profile, 0) wouldn't hurt...

Though I see that some profiles have multiple levels of nesting in /etc/apparmor.d. For example, given the following profile:

* /etc/apparmor.d/abstractions/ubuntu-browsers.d/chromium-browser

How would it be referenced in the profile string? Simply "chromium-browser" or "abstractions/ubuntu-browsers.d/chromium-browser"?

If the latter is possible, then maybe forbid just "../" and "/.." + call reject_metachars(profile, 0).

I've done some testing. As far as I tested, there is no way to make aa_stack_onexec load some custom profile that is not already loaded to the kernel. Even if you supply a path (like /tmp/something or /../../../tmp/something or ../../../tmp/something), it won't look up that path, it will seek the profile with such name (and to succeed, the profile must already exist and be loaded to the kernel, and the transition must be allowed by an AppArmor policy). Forbidding the slash can be a problem, since profiles generated by AppArmor tools (and a lot of profiles supplied by distributions) are in the form of path to the binary (like /usr/bin/man), and it would be impossible to transition to such profiles if the slash was disallowed.

The vim syntax file should also be updated to account for the new command
having an argument:

diff --git a/contrib/vim/syntax/firejail.vim b/contrib/vim/syntax/firejail.vim
index 9099a0808..0c8ebdbd8 100644
--- a/contrib/vim/syntax/firejail.vim
+++ b/contrib/vim/syntax/firejail.vim
@@ -52,7 +52,7 @@ syn match fjVar /\v\$\{(CFG|DESKTOP|DOCUMENTS|DOWNLOADS|HOME|MUSIC|PATH|PICTURES

 " Commands grabbed from: src/firejail/profile.c
 " Generate list with: { rg -o 'strn?cmp\(ptr, "([^"]+) "' -r '$1' src/firejail/profile.c; echo private-lib; } | grep -vEx '(include|ignore|caps\.drop|caps\.keep|protocol|restrict-namespaces|seccomp|seccomp\.drop|seccomp\.keep|env|rmenv|net|ip)' | sort -u | tr $'\n' '|' # private-lib is special-cased in the code and doesn't match the regex; grep-ed patterns are handled later with 'syn match nextgroup=' directives (except for include which is special-cased as a fjCommandNoCond keyword)
-syn match fjCommand /\v(bind|blacklist|blacklist-nolog|cpu|defaultgw|dns|hostname|hosts-file|ip6|iprange|join-or-start|mac|mkdir|mkfile|mtu|name|netfilter|netfilter6|netmask|nice|noblacklist|noexec|nowhitelist|overlay-named|private|private-bin|private-cwd|private-etc|private-home|private-lib|private-opt|private-srv|read-only|read-write|rlimit-as|rlimit-cpu|rlimit-fsize|rlimit-nofile|rlimit-nproc|rlimit-sigpending|timeout|tmpfs|veth-name|whitelist|xephyr-screen) / skipwhite contained
+syn match fjCommand /\v(apparmor|bind|blacklist|blacklist-nolog|cpu|defaultgw|dns|hostname|hosts-file|ip6|iprange|join-or-start|mac|mkdir|mkfile|mtu|name|netfilter|netfilter6|netmask|nice|noblacklist|noexec|nowhitelist|overlay-named|private|private-bin|private-cwd|private-etc|private-home|private-lib|private-opt|private-srv|read-only|read-write|rlimit-as|rlimit-cpu|rlimit-fsize|rlimit-nofile|rlimit-nproc|rlimit-sigpending|timeout|tmpfs|veth-name|whitelist|xephyr-screen) / skipwhite contained
 " Generate list with: rg -o 'strn?cmp\(ptr, "([^ "]*[^ ])"' -r '$1' src/firejail/profile.c | grep -vEx '(include|rlimit|quiet)' | sed -e 's/\./\\./' | sort -u | tr $'\n' '|' # include/rlimit are false positives, quiet is special-cased below
 syn match fjCommand /\v(allow-debuggers|allusers|apparmor|caps|deterministic-exit-code|deterministic-shutdown|disable-mnt|ipc-namespace|keep-config-pulse|keep-dev-shm|keep-fd|keep-var-tmp|machine-id|memory-deny-write-execute|netfilter|no3d|noautopulse|nodbus|nodvd|nogroups|noinput|nonewprivs|noprinters|noroot|nosound|notv|nou2f|novideo|overlay|overlay-tmpfs|private|private-cache|private-cwd|private-dev|private-lib|private-tmp|seccomp|seccomp\.32|seccomp\.block-secondary|tracelog|writable-etc|writable-run-user|writable-var|writable-var-log|x11)$/ contained
 syn match fjCommand /ignore / nextgroup=fjCommand,fjCommandNoCond skipwhite contained

Note: This is part of the new command checklist:

• https://github.com/netblue30/firejail/blob/53b2d817d5f412f0e4043bfea58cea39fd953cab/CONTRIBUTING.md#opening-an-pull-request.

@ChrysoliteAzalea commented on Jul 29:

I've done some testing. As far as I tested, there is no way to make
aa_stack_onexec load some custom profile that is not already loaded to
the kernel. Even if you supply a path (like /tmp/something or
/../../../tmp/something or ../../../tmp/something), it won't look up that
path, it will seek the profile with such name (and to succeed, the profile
must already exist and be loaded to the kernel, and the transition must be
allowed by an AppArmor policy). Forbidding the slash can be a problem, since
profiles generated by AppArmor tools (and a lot of profiles supplied by
distributions) are in the form of path to the binary (like /usr/bin/man), and
it would be impossible to transition to such profiles if the slash was
disallowed.

Thanks for testing; sounds like AppArmor does sufficient validation by itself
then.

Other than that, I have made some suggestions for the documentation.

And after we're done with the suggestions, could you rebase and squash/fixup
the commits in the branch and force-push? I would suggest squashing everything
into one commit and leaving only the following as the commit message:

Add support for custom AppArmor profiles (--apparmor=)

Example of using git rebase:

• Fix teams ignoring input sources e.g. microphones #4779 (comment)

Let me know if you have any questions.

Also how useful is firejail "--apparmor foobar" baz-program at all? Isn't it very likely that you type firejail --apparmor firefox-extra-hardened --private --net=eth0 firefox --new-tab and get a No executable firefox-extra-hardened found?

OK, sorry. Didn't think about that. I think, it's better to always use the --apparmor=<PROFILE> command line. Rolled the change back due to potential ambiguity.

Thanks, commited and rebased.

You need to rebase on top of master to resolve the conflict.

You need to rebase on top of master to resolve the conflict.

OK, rebased. It seems that other commits are shown in my pull request now, but the conflict is resolved.

@ChrysoliteAzalea commented on Aug 2:

You need to rebase on top of
master to resolve
the conflict.

OK, rebased. It seems that other commits are shown in my pull request now,
but the conflict is resolved.

It looks like you rebased master on top of your branch, rather than rebasing
your branch on top of master:

$ git log --pretty='%h %s' --oneline --graph \
  0ba8ed88b..master 0ba8ed88b..35cfd1553
* 35cfd1553 RELNOTES: add build and ci items
* f7b62f036 RELNOTES: add feature: Warn when encountering EIO during remount
* 9cd14a7e5 introduce new option restrict-namespaces
* d4f734ceb protocol filter: add x32 ABI handling
* 494dfc942 improve force-nonewprivs security guarantees
* fabf259d0 build: add autoconf auto-generation comment to input files
* 5687f0f7e ci: ignore git-related paths and the project license
* 79c91742f build: add dist build directory to .gitignore
* 6cbb0a6ec update m4 macro from autoconf-archive (2022.02.11)
* 582b1974b CI: keep old cppcheck job and ignore two files in new job that take too long to check
* 096738b6f CI: bump ubuntu to 22.04 and use newer compilers / analyzers
* 0c9c605c4 tests: disable calling curl in dns test, as systemd-resolved is used on CI runner
* 900b01b10 tests: try curl instead of wget for tracing dns resolution
* f3f3c1c9f tests: add alternative message for skipping test
* 9723de22c tests: drop checking for hosts file in trace test
* 583207c7a CI: fix wrong matching for test errors
* 38d9a802b Make list of paths const to fix a false positive of gcc analyzer
* 0877760bf zero-initialize two variables
* e2c224ffa CI: build all jobs with apparmor / selinux to cover more code
* b28423e07 Add support for custom AppArmor profiles (--apparmor=)
| * 74b5d24ba RELNOTES: add build and ci items
| * 86c0c2d50 RELNOTES: add feature: Warn when encountering EIO during remount
| *   06d3fd058 Merge pull request #5259 from smitsohu/ns
| |\
| | * 87afef810 introduce new option restrict-namespaces
| | * 214ac2084 protocol filter: add x32 ABI handling
| *   95f8cc7b8 Merge pull request #5271 from smitsohu/nnp
| |\
| | * 2cfd4dafc improve force-nonewprivs security guarantees
| *   f98675327 Merge pull request #5251 from kmk3/build-add-autoconf-comment
| |\
| | * 8fc604f5f build: add autoconf auto-generation comment to input files
| *   b90516d4a Merge pull request #5249 from kmk3/ci-ignore-git-paths
| |\
| | * f46b6c09d ci: ignore git-related paths and the project license
| *   00b2db8c8 Merge pull request #5248 from kmk3/build-gitignore-distdir
| |\
| | * 30d55f030 build: add dist build directory to .gitignore
| * a724bbd99 update m4 macro from autoconf-archive (2022.02.11)
| * 364a5659c Merge pull request #5275 from netblue30/ci_ubuntu_2204
|/|
| * 53f0b3950 CI: keep old cppcheck job and ignore two files in new job that take too long to check
| * cfc854788 CI: bump ubuntu to 22.04 and use newer compilers / analyzers
| * c971903de tests: disable calling curl in dns test, as systemd-resolved is used on CI runner
| * 4221b15f9 tests: try curl instead of wget for tracing dns resolution
| * b4f444486 tests: add alternative message for skipping test
| * e1cb7ce29 tests: drop checking for hosts file in trace test
| * 057f431b0 CI: fix wrong matching for test errors
| * eb20f52ef Make list of paths const to fix a false positive of gcc analyzer
| * e47bc3bc1 zero-initialize two variables
| * 3a5954c12 CI: build all jobs with apparmor / selinux to cover more code
|/
* 89441e48e Deny Tor related profiles access to /sys/class/net

Usually all that is needed to rebase to the current master is the following:

# update master
git checkout master
git pull
# rebase mybranch onto master
git checkout mybranch
git rebase -i origin/master

Try that and on the rebase screen, delete the lines of commits not related to
your branch (the "restrict-namespaces" commit in this case) and proceed as
usual, resolving the conflict (by keeping apparmor on the final version of
the syn match line).

Also, git caught a trailing tab, which should be removed:

$ git diff --check master
src/firejail/profile.c:946: trailing whitespace.
+	

Other than that, the branch should be good.

OK, sorry. Re-rebased the branch and force-pushed again.

@ChrysoliteAzalea commented on Aug 2:

OK, sorry. Re-rebased the branch and force-pushed again.

No problem; it looks good now.

Ah I forgot to check locally and spoke too soon.

It had an extra merge for some reason, but I did a rebase locally and force
pushed.

Now I'm sure it's fixed.

@ChrysoliteAzalea Sorry, I didn't notice that the source branch is also named
"master"; that was likely the issue with the rebase, as trying to rebase master
onto itself likely results in a no-op.

It might have worked with the following steps:

git fetch
git checkout master
git rebase -i @{u}

Anyway, I would suggest always using a topic branch rather than committing
directly on master (especially for non-trivial changes), as it should make
organization easier.

Lastly, while the branch on the PR is already fixed, your local master branch
might not be. In which case, try the following commands to reset it:

git stash # just in case
git fetch
git checkout master
git reset --hard @{u}

OK, done. Thank you.

Force-pushed in order to sign my commit. No changes to the contents of files have been made.

merged, thanks!